Don't stop pools #1199
Don't stop pools #1199
Conversation
shsms
requested review from
ela-kotulska-frequenz
and removed request for
a team
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
src/frequenz/sdk/timeseries/pv_pool/_pv_pool.py:181
- Removing this cleanup call might leave pending tasks or open channels. Consider verifying that _pool_ref_store's resources are managed appropriately elsewhere, and document the reasoning for this removal.
await self._pool_ref_store.stop()
src/frequenz/sdk/timeseries/ev_charger_pool/_ev_charger_pool.py:219
- Ensure that omitting the stop operation on _pool_ref_store doesn't lead to resource leaks. If resource cleanup is handled in another part of the code, an inline comment explaining this would improve clarity.
await self._pool_ref_store.stop()
src/frequenz/sdk/timeseries/battery_pool/_battery_pool.py:401
- Double-check that the removal of the stop call does not leave orphaned tasks or channels. Consider adding tests or documentation to confirm that resource management remains intact.
await self._pool_ref_store.stop()
There was a problem hiding this comment.
Wow but the formulas are shared, too.
So we have the same problem if 2 actors uses the same formula (like BatteryPool.power()) and one of them stops it.
And the same for microgrid.producer().power - one actor stops it, it won't work for other actors.
So this PR won't fix the problem :/
Maybe quick fix would be to not share pools & formulas? (I don't know how much work it would be).
Or have counter (how many is active and how many stopped
| async def stop(self) -> None: | ||
| """Stop all tasks and channels owned by the EVChargerPool.""" | ||
| await self._pool_ref_store.stop() |
There was a problem hiding this comment.
Could you please add comment why this methods are empty?
It looks like bug now, because method description and definition does different thing.
|
👋 reference counting 👋 |
When there are multiple pool instances controlling the same components and one of them stops, that would stop all the pools. This commit adds an emergency fix to prevent this bug, to buy some time to come up with a better solution. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
github-project-automation
bot
moved this from To do
to Review approved
in Python SDK Roadmap
This is not a problem. Users only want to close the resources that they have created. So when they make a We don't have the idiom of holding references to built-in formulas. When people create custom formulas, they can stop those; that's fine. Also, when formulas are closed by mistake, it's immediately noticeable. But when the battery pool is closed by mistake and we're only subscribing to SOC or capacity, for example, it's much more dangerous because we'll just assume the value hasn't changed.
Because there are so many metrics, that would quickly become a noticeable overhead. Additionally, the pools share access to the power manager, etc. I think we should just look for a proper solution, hopefully next week. |
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
3 participants
When there are multiple pool instances controlling the same components and one of them stops, that would stop all the pools.
This PR adds an emergency fix to prevent this bug, to buy some time to come up with a better solution.